-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ESLint Plugin: Introduce rule json-schema-no-plain-object-types
#21687
Conversation
Do we actually validate this? To clarify, I don't necessarily see it as a blocker, since this effort can be future-compatible to some future enhancement to implement the validation. |
Size Change: 0 B Total Size: 841 kB ℹ️ View Unchanged
|
It's a common point of strain in the implementation of these custom rules, but: Are "blocks" a thing that The answer may very well be "Yes". Some of the existing rules could similarly qualify as being pretty WordPress-specific (i18n rules, for example). |
@mcsf I was thinking about this really but I don't know why attributes schema model wasn't structured like JSON schema model from first. It looks like custom schema built for Gutenberg rather than JSON schema.
But what if I would like to have the
and in registration, we force |
However, I believe my suggestion is so late because how the things got structured already, there won't be such a flexibility.
Extracting the |
As discussed: on the client we don't, though on the server the WP API does validate. Whatever we build in the future to actually use validation, I personally think there is already a benefit in requiring the developer to be explicit in data needs. |
I battled with this myself, and I think you can tell in the fact that I zigzag between mentions of "JSON schema" (dubious) and "block attributes" (also dubious because the rule obviously doesn't actually analyze the declarations). Keeping small in scope and generic enough to be compatible with the WP API can help justify that it remain a part of a WP set of rules, as you suggest:
|
Hi, @muhammedmagdi —
Indeed, though the attribute model in Gutenberg was influenced by JSON Schema, it was shaped by more pressing concerns around attribute sourcing, concerns that JSON Schema might not have responded to. Roughly, the only things that mattered for an attribute would be its base type and, if any, its default value; internally, the only validation really happening was the casting of values to the base type. Meanwhile, being able to declare complex sourcing of attributes was a much more powerful feature for Gutenberg. It allowed attributes to be sourced from the actual block markup, thereby avoiding the need to duplicate data between the data encoded in the block boundary and the markup. It also allowed the use of completely different sources: an attribute may live in a post's metadata, for instance.
Attributes themselves shouldn't need the This doesn't preclude some sort of
Correct, the current schema is here to stay, even if we allow optional enhancements in the future. JSON Schema is a very expressive system which, if fully taken advantage of, would likely be "too much" for Gutenberg. What prompted this pull request was the observation that, over time, blocks that grow in complexity tend to escape Gutenberg's attribute validation system by introducing "wildcard" attributes that can contain anything. We can mitigate this by requiring arrays to specify |
In the same vein, it would be great to add a similar check for ensuring that |
@TimothyBJacobs: if agree with the general direction, then yes, arrays should be addressed too. What do you think about #21687 (comment) and #21687 (comment)? I personally think it's a worthwhile change in itself: it both guides developers and requires something of them, even if the editor doesn't validate the data. |
What’s the status of this PR? 😃 |
It's been a while. Personally, do you think it has value? This was a nice thing to try out, but I don't feel strongly about getting it in. :) |
I did another check for this PR and I found one fundamental issue with this proposal. While I'm personally very much in favor of having this type of validation added, it wouldn't be useful in the actual form simply because we have all block definitions in |
It makes sense to target
|
Rather than using ESLint, we could run JSON schema validation on the full block definition inside |
See: https://www.npmjs.com/package/eslint-plugin-json#is-eslint-really-the-best-tool-to-lint-my-json
|
Thanks, that makes sense. |
This feels very low-priority to me, and our PR count is high enough as it is, so I'm going to close it. Anyone should feel free to reopen if they want to pursue this. Thanks for the debate. |
Description
Require all "object" types within a block's attributes to define their expected properties (
json-schema-no-plain-object-types
).Block attributes must conform to types as specified in the WordPress REST API documentation, a behavior based on JSON Schema. This schema is used to validate attribute data not only in the REST API, but also in the block editor.
This rule requires that any declaration of an
object
type specify the exact properties that make up the object. This applies to the type of attributes themselves as well as any data within an attribute, such as when an attribute of typearray
expects values of typeobject
.Status
This is an early exploration and not necessarily something to carry through. However, I think the conversation around how strictly we encourage blocks to specify their data models is important. A recent conversation in #21359 (comment) with @aduth sparked this PR. The question of whether violations of this rule should be seen as errors is up for discussion too.
As I haven't written rules for
eslint-plugin
before, I am sure I am forgetting things in this PR. So far:How has this been tested?
Addition of rule-specific unit tests.
Screenshots
Types of changes
Checklist: